-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(parsed_transaction_history): add transaction pagination #14
feat(parsed_transaction_history): add transaction pagination #14
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, thank you for your contribution! That was a fast turnaround, from raising the issue in Discord to making a PR! I've left a few minor comments, but this is a solid PR overall!
Can you also please run the cargo fmt
command before you commit your new changes? That way everything is formatted properly and all the checks for the build pass
src/enhanced_transactions.rs
Outdated
@@ -31,14 +31,20 @@ impl Helius { | |||
/// | |||
/// # Arguments | |||
/// * `address` - An address for which a given parsed transaction history will be retrieved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to update this to reflect the new request type as the address
argument will no longer be valid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parse_transactions follows this format as well (showing the parameters of the type) which is why I framed it that way. Would you like me to update that one as welll?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, that's an oversight on my end — parse_transactions
should have ParseTransactionsRequest
as its argument and the description should stay the same
@@ -210,6 +210,12 @@ pub struct ParseTransactionsRequest { | |||
pub transactions: Vec<String>, | |||
} | |||
|
|||
#[derive(Serialize, Deserialize, Debug)] | |||
pub struct ParseTransactionHistoryRequest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer if we renamed it to ParsedTransactionHistoryRequest
so it follows the same name as its function with request added at the end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks, and congrats on your first open-source contribution! 🔥
Added ability paginate the results of the request in the transaction history, by doing the following:
address
String and an Optionbefore
Stringparsed_transaction_history
to input the new type, and update the API call ifbefore
exists. Updated documentation above it.I've never contributed to an open-source project, so please forgive me if this is the wrong process for suggesting edits or updates to the module.
I looked through the code to adhere to similar structures that y'all used in other aspects, but either way this should be a complete fix here or a good start to this implementation.
Any feedback or comments please let me know. Otherwise it should be good to go.
Cheers,
rhh